test: refuse to measure a binary that was not built from this source - #898
Conversation
|
🤖 Generated with Claude Code |
…r runs it
jd's design: one controller arm per batch of tests, so a stale .so can never be
measured and nothing has to rebuild per test.
TWO QUESTIONS THE HARNESS COULD NOT ANSWER.
selftest 110 compares the INSTALLED .so against the one built in this tree, so a
missed install and a foreign overwrite were already caught. Nothing derived anything
from the SOURCE TEXT, so both copies could agree with each other while both were
stale against edited source. PGC_SKIP_BUILD opens that hole widest, because not
rebuilding is its whole purpose.
And a cp is not enough. shared_preload_libraries maps the library at postmaster
start, so make install over a running instance changes the file and nothing else:
every backend keeps executing the code it already mapped. A binary can match the
source exactly while the server runs something older.
THE SHAPE. Whoever builds records a fingerprint of the build inputs -- src/*.c,
src/*.h, the Makefile, the control file, the shipped SQL. Every suite in the batch
recomputes it and compares, which is one build per batch and one hash per suite. Then
once the cluster is up, the suite compares the binary's mtime against
pg_postmaster_start_time().
-- source: 28b66bd0ac0c matches the binary under test
-- server: started after the binary was installed
A stale source fingerprint and a server predating the binary are both FATAL, because
every check that followed would be about code that is not running. A missing stamp is
UNVERIFIED and said plainly rather than failed: a person who ran make install by hand
has no stamp, and refusing would break a documented workflow.
MEASURED, three arms:
normal build, install, run source matches, server fresh, PASSED
source edited + PGC_SKIP_BUILD=1 FATAL: not built from this source, exit 1
binary newer than the postmaster FATAL: server already running, exit 1
The two verdict functions are pure and take their inputs as arguments, for the same
reason pgc_build_needs_clean does, so selftest 340 exercises fresh, stale, unknown,
predates and the non-numeric cases without a build. It also requires that the
fingerprint MOVES when a build input moves, STAYS when nothing does, and ignores a
file that is not a build input -- a fingerprint that never changes reports fresh
forever, which is this file's own failure mode one level down.
WHAT WROTE THE STAMP IN THE WRONG PLACE, AND WHAT CAUGHT IT. The first revision wrote
it in the skip-build branch, so every run recorded the source it was about to compare
against and a suite measuring an edited tree reported "matches the binary under test".
The arm that requires `stale` is what caught it. The stamp is now written only where
the install succeeded, and the comment there says why.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…lt from jd's ask: a controller arm that verifies the build is fresh against the batch of tests being run, so we never measure a stale .so but also do not rebuild for every test. run_all_versions.sh is that controller. It builds and installs once per major and then runs every suite with PGC_SKIP_BUILD=1, so the suites have no way of their own to tell whether the binary they measure came from this tree. It now records the fingerprint of the build inputs after a successful install, and lib.sh checks it in every suite whether that suite built or skipped. In a subshell sourcing lib.sh rather than recomputing the hash inline: two implementations of one fingerprint drift, and the suites compare against exactly what this writes. Not `|| true`. If the stamp cannot be written, every suite in the batch reports "freshness UNVERIFIED" and the controller silently stops being a controller -- the batch degrades to the state this exists to prevent, with nothing saying so. A failure now prints what it means for the run below it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
pgc_setup writes .pgc_source_stamp.<major> beside the source it fingerprints, so running any suite from a checkout left an untracked file in `git status`. This project gates on a clean tree, so a harness artifact that dirties one is a recurring false alarm rather than a cosmetic issue. Placed next to .pgc_built_for_major, which is the same kind of file written for the same reason. Verified the new rule is the one that matches -- the file was NOT ignored before, and `git check-ignore -v` now names .gitignore:10 -- and that the existing rule still matches its own file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Same omission as commandprompt#897: this project records test-infrastructure changes in CHANGELOG.md and I opened the PR without an entry. The entry says what both checks refuse, and says that neither fails when it cannot answer -- an unstamped tree prints "freshness UNVERIFIED" and names the question it did not answer, rather than printing nothing and reading as a pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
|
Note for whoever merges second: #897 and #898 both edit the same block of #898 writes the source stamp inside #897 extracts that same branch into Git will merge these without a conflict in at least one order, and the result would be wrong in a way no test would catch: the stamp write must stay inside the extracted function, after the install that succeeded. If it ends up outside, it either stops being written for bash suites, or gets written on a path that did not build — which is precisely the tautology #898's own comment records catching once already. Whichever lands first, the second should be rebased with the stamp write placed inside No action needed on this PR right now; recording it so it is not discovered at merge time. |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed at c6e20581. The mechanism works and the matrix path is genuinely protected — I
reproduced your red arm exactly. Three gaps, all in what the fingerprint does not see or where the
check does not run. Requesting changes because the failure mode of each is a false assurance
rather than an absent one, and this PR exists to remove exactly that.
1. The fingerprint does not cover objstore/, and that module is separately built and installed
pgc_source_fingerprint reads $dir/src/*.{c,h} at maxdepth 1 plus the top-level Makefile,
*.control and *.sql. objstore/columnar_objstore_module.c is a build input — Makefile:132-143
runs $(MAKE) -C $(OBJSTORE_DIR) for all, install and clean, producing and installing
pgcolumnar_objstore.so.
Demonstrated, with a control so this is the gap and not a broken probe:
baseline -- source: 28b66bd0ac0c matches the binary under test
edit objstore/columnar_objstore_module.c -- source: 28b66bd0ac0c matches the binary under test <-- stale
edit src/columnar_projection.c (control) FATAL: the binary under test was not built from this source
source now 8e07a5495a31, binary built from 28b66bd0ac0c
restored -- source: 28b66bd0ac0c matches the binary under test
So objstore_module, objstore_sink_write and objstore_stash_recovery can measure a stale
module while the suite prints an explicit assurance that the binary matches. That is worse than
printing nothing, because the line is what stops the next person checking.
The fix is one find line. Whichever way you extend it, the property worth asserting in the
selftest is "every directory the Makefile builds from is in the fingerprint" rather than a list
that has to be remembered — objstore/ was added once and will not be the last.
2. The postmaster arm cannot fire through any shipped path
pgc_setup is unconditional:
PGC_WORKDIR="$(mktemp -d /tmp/pgcolumnar-test.XXXXXX)"
PGC_PGDATA="$PGC_WORKDIR/data"
... initdb -D "$PGC_PGDATA" ... pg_ctl -D "$PGC_PGDATA" ... start
Neither variable is overridable and nothing in test/ supplies a datadir, so every suite
initdbs a fresh cluster and starts it after the .so was installed. _pm_epoch is therefore
always greater than _so_epoch, the verdict is always fresh, and predates is unreachable.
test/selftest/340 tests pgc_running_binary_verdict as a pure function with fixture values
(2000, 1000 -> predates). That proves the arithmetic and not the call site — this project's own
rule: a suite that sources a helper cannot see whether anything calls it; feeding a function
fixtures proves its arithmetic and nothing else, so the caller can be deleted with every check
still passing.
I am not saying delete it. The hazard is real and the message is the best one in the PR. But as it
stands the tree carries a check that cannot report the condition it names, and the selftest reads
as though it does. Either name a path where a cluster outlives a reinstall and give it an arm, or
say in the comment that this arm guards a workflow the suites themselves never take.
3. devloop.sh — the documented dev loop — gets UNVERIFIED, not enforced
This is the one I would fix first, because it is the loop a human uses while editing C.
$ devloop.sh ... drop_cleanup
-- source: 28b66bd0ac0c, freshness UNVERIFIED (no stamp for major 18)
devloop.sh:92 runs the suites with PGC_SKIP_BUILD=1 after building itself, and only
run_all_versions.sh:725 writes the stamp. So the whole class this PR exists to catch — edit a
file, forget to rebuild, measure the old binary — is unprotected in precisely the loop where it
happens, and the unknown branch prints and continues by design.
devloop.sh already builds and installs, so it can write the stamp in the same place, and it is
the natural owner: it is the other thing in the tree that installs before running suites.
What is right
The red arm reproduces exactly, and naming both fingerprints in the message is the detail that
makes it debuggable rather than merely loud — source now X, binary built from Y tells a reader
whether they are looking at drift or at a broken fingerprint, which "stale" alone does not.
unknown not being a failure is the correct call for the hand-make install workflow, and the
selftest covers all four verdicts in both directions including the non-numeric case.
And your two disclosed self-findings are the kind that matter: the stamp write being || true
would have left every suite reporting freshness UNVERIFIED silently — the exact state the arm
exists to prevent — and the untracked stamp tripping the clean-tree rule. Verifying that the NEW
.gitignore rule is the one matching, rather than assuming, is the right instinct; it is the same
shape as a mutation you have to prove applied.
One note on scope
test/lib.sh is sourced by every suite, so I agree with running the full matrix on both majors
rather than a reduced set — there is no equivalent here of the CHANGELOG.md-only argument that
justified the reduced re-gate on #892.
All three from @jdatcmd's review. Each failed the same way: the check reported an assurance it had not earned, which is worse than reporting nothing, because the line is what stops the next person looking. THE FINGERPRINT DID NOT SEE objstore/. It read $dir/src only, and objstore/columnar_objstore_module.c is a build input -- the top-level Makefile builds and installs it by recursion as a separate shared library. Editing it left the fingerprint unchanged, so objstore_module, objstore_sink_write and objstore_stash_recovery could measure a stale module while the run printed "matches the binary under test". Reproduced with his control: baseline 1449dc9dba17 edit objstore/columnar_objstore_module.c c85763de5bb5 CHANGED edit src/columnar_projection.c (control) 4fffcff2b65f CHANGED edit objstore/Makefile 75d57e06199d CHANGED restored 1449dc9dba17 back to baseline Naming objstore/ would fix today and fail the next time a module is added, so pgc_source_build_dirs DERIVES the set: every directory with its own Makefile, the same rule the build follows. The selftest asserts the property rather than the list -- it parses the `$(MAKE) -C` recursion out of the Makefile and requires every target to be covered -- with a premise check that the parse found something, because a guard that found nothing to check has abstained rather than passed. Proved by removal: reverting to src-only reddens both arms with `missing: objstore`. THE POSTMASTER ARM COULD NOT FIRE THROUGH ANY SHIPPED PATH. pgc_setup always initdb's a fresh cluster and starts it after the install, so the postmaster is always newer than the .so and `predates` was unreachable. selftest 340 fed the verdict function fixture values, which proves its arithmetic and not its call site: the call could have been deleted with every check still passing. pgc_check_running_binary is now extracted from pgc_setup and takes the library path as an argument. The selftest points it at a file it has just touched, so the stat, the pg_postmaster_start_time() query, the verdict and the refusal all run for real against the live cluster and only the path is redirected -- nothing has to touch the installed library to prove the guard fires. Proved by removal: neutering the `return 1` reddens "a library newer than the running server is REFUSED" with got [0] want [1]. devloop.sh GOT UNVERIFIED, NOT ENFORCED, and it is the loop a human uses while editing C. It builds and installs and then runs suites with PGC_SKIP_BUILD=1, and only run_all_versions.sh wrote the stamp -- so edit-a-file-and-forget-to- rebuild was unprotected in exactly the place it happens. devloop is a controller and now records what it installed. Measured end to end: A devloop, clean tree -- source: 1449dc9dba17 matches the binary under test B edit C, no rebuild exit 1, FATAL, source now c636277e827f, binary built from 1449dc9dba17 C restore, same command exit 0, matches, PASSED pgc_major_of is extracted alongside it because devloop writes a stamp whose PATH is keyed on the major and pgc_setup reads it back. Two copies of that sed would be two answers to "which major", and the failure would be a stamp written where nothing looks for it: a silent UNVERIFIED rather than an error. harness_selftest goes from 277 checks to 288. One process note. My first removal proof for the objstore fix SILENTLY FAILED TO APPLY -- nested-heredoc escaping -- and printed PASS. The assert inside the mutation caught it. Without that I would have reported a guard as proven when it had never been exercised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
c6e2058 to
9058515
Compare
|
Reworked at 1. The fingerprint now derives its build directoriesNaming The selftest asserts the property you asked for rather than a list: it parses the Proved by removal — reverting 2. The postmaster arm can now fire, and is drivenYou were right that feeding the verdict fixture values proves its arithmetic and not its call site — that is this project's own rule and I had shipped exactly what it warns about.
Six arms: refused when newer, the message naming the restart, accepted when older, saying so rather than staying silent, and an unreadable path reading Proved by removal — neutering the That is the arm that could not exist before. 3. devloop.sh writes the stampIt is a controller — it builds and installs, then runs suites with
Verification
Full matrix rather than a reduced set, and I agree with your note on why: One process note against myselfMy first removal proof for the objstore fix silently failed to apply — nested-heredoc escaping — and printed Still standingThe |
… SC1087)
`$_bd_var[[:space:]]` reads to shellcheck as an array expansion, so
`shellcheck -S error` -- a CI-only job the local gate never runs -- failed
on the selftest arm added in the last push. Braced to `${_bd_var}`.
Semantically identical in bash; proved by running the suite rather than by
inspection. harness_selftest.sh: 288 passed + 0 failed + 0 unrunnable,
including `PREMISE the Makefile's recursion was actually parsed`, which is
the check that goes red if the sed stops resolving the variable name.
`shellcheck -S error -s bash test/*.sh test/selftest/*.sh` now exits 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
|
CI red on my last push, fixed at
Proved rather than asserted, because "semantically identical in bash" is exactly the kind of claim that is right until it isn't:
The gap this exposes: I also grepped the class rather than just the instance: The two |
linuxhikerpm
left a comment
linuxhikerpm
left a comment
There was a problem hiding this comment.
Reviewed exact head b4a77c923eb4ac74253c65d1c5d41ed67b5cd75e in an isolated worktree. harness_selftest.sh is 288/288 green on PG18.6, but three false-freshness paths remain. I reproduced all three rather than inferring them.
1. The source-stamp writer cannot report failure
test/lib.sh:708-710:
pgc_write_source_stamp() {
printf '%s\n' "${2:-}" > "${1:-/dev/null}" 2>/dev/null || true
}Both controllers wrap this function in if (...) and promise to warn when it fails (test/run_all_versions.sh:726-736, test/devloop.sh:95-105), but || true makes that branch unreachable.
Driven against an unwritable target on this head:
test/lib.sh: line 709: /proc/pgc-source-stamp: No such file or directory
write_rc=0 exists=no
So the stamp is absent, the controller reports no warning, and every child suite degrades to UNVERIFIED despite the comments explicitly saying this was fixed. Remove the status suppression and add an arm that drives the actual writer against an unwritable destination.
2. The fingerprint hashes concatenated contents without paths or boundaries
test/lib.sh:612-624 sorts filenames, then runs xargs -0 cat | md5sum. The hash therefore cannot distinguish changes that preserve the concatenated byte stream while moving bytes between translation units.
I created two source files:
/* a.c */ static int x=1;
/* b.c */ static int x=2;Both compile separately. I then moved b.c's bytes into a.c and emptied b.c. The new source fails to compile with a redefinition, yet the fingerprint is unchanged:
before_hash=d2d6c3a12720 after_hash=d2d6c3a12720
initial_compile=0 repartitioned_compile=1
error: redefinition of ‘x’
A skip-build run consequently prints matches the binary under test for source that cannot produce that binary. Feed each relative path and an unambiguous length/delimiter into the digest, not only file contents, and mutation-test the boundary case.
3. “Keyed by major” aliases distinct installations of the same major
test/lib.sh:717-730 says one tree installs into several prefixes and each has its own binary, but pgc_source_stamp_path DIR MAJOR stores only .pgc_source_stamp.18. Two PG18 pg_configs with different pkglibdirs resolve to the same stamp:
config_a=18 config_b=18
stamp_a=/tree/.pgc_source_stamp.18
stamp_b=/tree/.pgc_source_stamp.18
This container itself has multiple PG18 installations, so the case is not hypothetical. Building current source into prefix A updates the shared stamp. A subsequent PGC_SKIP_BUILD=1 run against stale prefix B sees the current source fingerprint, while the postmaster check also passes because its newly started server is newer than B's old .so. The run then explicitly reports fresh while executing B's stale binary.
Key the record by installation identity as well as major (at minimum the normalized installed-library path or pg_config/pkglibdir) and add a two-config arm. As written, the comment states the distinction the key discards.
|
CI finished on Reading the breakdown rather than a bare count, because a pending check reports conclusion For completeness on what the previous run did and did not say: at Nothing outstanding from my side on this PR. It is waiting on your re-review, and on the |
…ts were stale Found reviewing my own PR before asking for another look. THE ENTRY WAS UNDER A RELEASED SECTION. It sat at CHANGELOG.md:225, under `## [1.0-alpha3] - 2026-09-02`, not under `## [Unreleased]` at line 17. So this PR, which is not merged, claimed the freshness controller shipped in a release tagged a week ago. A reader of the alpha3 notes would have believed it was in the tarball they have. Moved to [Unreleased]. BOTH COUNTS IN IT WERE STALE, and both were stale by the same cause: the entry was written before the review, and the rework that answered the review grew what it describes without the entry moving. "16 arms" -> 27 (grep -c '^check ' on selftest 340) "goes from 261 checks to 277" -> 288 Measured rather than derived, because a count is a claim: main f2af080, a fresh worktree accounting: 261 passed + 0 failed = 261 this branch accounting: 288 passed + 0 failed = 288 I nearly published 288 - 27 = 261 as the baseline instead of measuring it. That subtraction assumes selftest 340 is the only thing in this PR that changes the count, and several arms in this suite SWEEP the tree rather than stating a fixed number of checks, so a lib.sh change can move a count without adding a `check` line. The arithmetic happened to agree with the measurement; it was not entitled to. The sentence now also names the three arms the review added -- the derived build-directory set, the running-binary check driven against a live cluster, and devloop writing the stamp -- because "27 arms" with no account of where 11 of them came from is a number a reader cannot check. Verified after the change: docs_style 9 checks PASSED (it reads CHANGELOG.md at two sites, so it runs whole; the em/en dash arm and the VERSION-citation arms both pass) harness_selftest 288 passed + 0 failed + 0 unrunnable PASSED shellcheck -S error -s bash test/*.sh test/selftest/*.sh exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
|
I reviewed this myself before asking you to look again. One defect, three parts, fixed at The entry was filed under a released versionIt sat at Moved to Both counts in it were stale, from the same causeThe entry was written before your review; the rework that answered the review grew what the entry describes, and the entry did not move.
Measured, not derived: I nearly published The sentence now also names where the extra 11 arms came from — the derived build-directory set, the running-binary check driven against a live cluster, and devloop writing the stamp — because "27 arms" with no account of the growth is a number a reader cannot check. Why I am reporting a changelog fix as a findingBecause it is the same defect this PR exists to prevent, one level up. The PR's whole argument is that a stale artifact under an explicit assurance is worse than one under no assurance, since the assurance is what stops the next person checking. A changelog entry filed under a shipped release is exactly that: a specific, confident, checkable claim that is false, sitting where a reader goes instead of checking. I would have flagged it in someone else's PR. Verified after the change
Still outstanding, unchangedThe |
|
For whoever merges #897 and #898 second: they touch the same block of I tested the merge rather than reasoning about it. One correction to how this was first described The conflict is this, verbatim from the merged tree: The hazard survives the conflict, and one detail makes it worse than an ordinary tricky The pytest harness calls And the comment at the conflict site argues for the wrong resolution. "The stamp is written The resolution: the stamp write goes inside This also subsumes one of my review findings on #898: Tested against |
Two changes from the #902 review, both from OffgridwithJD. CONTEXT.md's twin rule now says to pin the SHA the twin was tested against rather than the branch name. Their argument is the one that convinced me: a branch name is not checkable later, and it is why they could verify my claim at all. The harness branch moved three times while the first twin was being written, and two of those moves changed its content -- so "blocked on #897" and "blocked on #897 at b785795" are different claims and only one can be falsified. Same reason a tag is read from the API rather than from a local ref, which I got wrong earlier today and filed a false issue over. The twin's header records that #897 moved a fourth time, to 9064a46, and DELIBERATELY DOES NOT UPDATE THE PIN. The point of a SHA is to say what was tested. What is recorded instead is why the pin still describes the current head, verified here rather than taken from the push notice: b785795 test/pytest tree = b20ad7e 9064a46 test/pytest tree = b20ad7e whole delta = 30 lines in one test/selftest/ file the harness never reads NOT CHANGED, deliberately: the five x86_64 build failures on this PR are the PGDG apt mirror, not this branch. The mirror is serving a Release file created at 17:16:59 alongside a component index last modified at 09:41:12, so the index cannot match the manifest describing it. Two attempts twenty minutes apart produced byte-identical hashes, which rules out a race. #898 at 6939bba and #897 at b785795 both went fully green before 17:16 and both #897 at 9064a46 and this branch fail after it, with #897's delta being thirty lines in a directory no build job reads. aarch64 passed all five majors throughout. Patching ci.yml around a mirror that is mid-sync would outlive the outage and get copied. docs_style.sh: 9 checks, PASSED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approving at 6939bba. All three findings are closed, and I verified the load-bearing one with
the same probe that found it rather than reading the diff.
The objstore gap, closed and re-proved
My probe now refuses where it previously passed silently:
baseline -- source: 1449dc9dba17 matches the binary under test
edit objstore/columnar_objstore_module.c FATAL: the binary under test was not built from this source
source now f9951b59e2a1, binary built from 1449dc9dba17
And the fix is structural rather than a list. pgc_source_build_dirs derives the set with
find -mindepth 2 -maxdepth 2 -name Makefile, so any directory that carries its own Makefile is in
the fingerprint. objstore/ was the one that existed; the next one is covered without anybody
remembering. That is the property I asked for rather than the patch I suggested.
The unreachable arm, fixed the right way round
I said predates could not fire through any shipped path, and that selftest/340 feeding
pgc_running_binary_verdict fixture values proved its arithmetic while the call site could be
deleted with every check passing.
The answer was not to invent a reachable path or to delete the arm. pgc_check_running_binary
now takes the .so path as an argument, so the selftest points it at a file it has just touched
and the stat, the query, the verdict and the refusal all run for real — only the path is
redirected. Nothing has to disturb the installed library to prove the guard fires.
And the comment states the limit rather than hiding it: through any shipped path the postmaster is
always newer than the .so, so predates remains unreachable in normal operation. Recording that
is worth more than the arm.
devloop.sh
Now writes the stamp, sources lib.sh in a subshell so there is one fingerprint implementation
rather than two that drift, and is explicitly not || true — which was your own earlier
self-caught bug in this same PR, applied to the new site before I could raise it.
That was the finding I said I would fix first, because it is the loop a human uses while editing C.
One thing to carry into the merge
The hazard I posted on both PRs stands and matters more now that the pytest harness drives
pgc_build_and_install directly: whoever merges second must put the stamp write inside that
extracted function, not leave it in pgc_setup. Git conflicts rather than merging cleanly, so a
human is forced to look — but the comment at the conflict site argues for the wrong resolution.
Verification
12 of 12 on 6939bba, breakdown read rather than the colour, including both suites legs. That
run predates the PGDG mirror going inconsistent at 17:16, so it is a real green and not a stale one.
Approving. This is a better change than the one I reviewed, and the parts I did not ask for — the
derived build-dir set and the drivable call site — are the parts that will still be right after the
next directory is added.
|
You are right about the comment, and I am not pushing the fix without you deciding — because a push here would inherit your approval. The comment does argue for the wrong resolution
"written HERE and nowhere else" was meant as a statement about which branch — not the skip-build one. At the conflict it reads as a statement about which location, and the resolver's question is exactly "does this line stay in A comment that is right about the past and wrong about the future, sitting precisely where someone is forced to make a decision. Worse than no comment. Proposed replacementSame fact, stated as a property of the path rather than of the position, so it survives the extraction and argues for the correct resolution at the conflict. Why I am asking rather than pushingThis repository does not dismiss stale reviews. Your APPROVED at Three ways to take it, and I am happy with any:
On the merge orderAgreed that it conflicts rather than merging cleanly, and that this is the good outcome. One thing to add for whoever resolves: after either order, the check that catches a wrong resolution is not in the diff — it is running the pytest corpus and seeing whether it prints Thanks for re-running the objstore probe rather than reading the diff. The |
|
Cross-posting the verified resolution for the Short version: I trial-merged the two branches locally. One conflict, one hunk, and the obvious resolution ("take HEAD, it is the refactor") silently deletes the stamp write. The correct one moves it inside Nothing pushed to either branch; it was a dry run. Your approval at (Edited: the first line originally read "for the interlock" — I passed this body inline to |
…at was false (commandprompt#432) THREE ITEMS, from two reviews I had not read when I asked for a re-review. I posted a closure table against @jdatcmd's 14:45 review while his 18:05 one and @linuxhikerpm's 16:38 one were both sitting on the PR. That is my error and it is the reason this commit exists rather than an approval. --- @linuxhikerpm 1: AN objstore/ EDIT WAS CERTIFIED AS ALREADY BUILT --------- Reproduced against the branch before fixing: objstore_before=2799803eaeac objstore_after=2799803eaeac builds=1 second=already-built source_fingerprint() read src/*.c, src/*.h, the top-level Makefile, *.control and *.sql. objstore/ is a SEPARATELY BUILT shared library the top-level Makefile reaches by recursion, so editing objstore/module.c left the hash unchanged and build_once treated a stale module as current. Its docstring said "Same input set as pgc_source_fingerprint in test/lib.sh", which was itself false -- and this is an INDEPENDENT implementation, so rebasing commandprompt#898 would not have fixed it. THAT is the argument for the two becoming one implementation rather than two that happen to agree. source_build_dirs() now derives the set by the rule the build itself follows: src/, plus any directory carrying its own Makefile. Naming objstore/ would fix today and fail the next time a module is added. AND A COLLISION THE GLOB DOES NOT FIX. The hash mixed in each file's bare NAME. With two build directories src/module.c and objstore/module.c become interchangeable -- swap their contents and the fingerprint does not move. It now mixes in the path relative to the tree. --- @linuxhikerpm 2: make_cluster LEAKED ITS TREE ON A FAILED SETUP ----------- make_cluster_error=RuntimeError new_roots=1 leaked=['/tmp/pgc-pytest-777-h3phhtxc'] root came from mkdtemp and then Cluster(), initdb(), start() and is_ours() ran with no cleanup guard. conftest.py cannot clean up after it, because `cluster, root = make_cluster(...)` never completes when the call raises. The HANDLED is_ours() path leaked too: it stopped the cluster and left the directory. Every exit that is not a successful return now stops whatever was started and removes the tree. It catches BaseException rather than Exception, because a KeyboardInterrupt during initdb leaks a datadir and a possibly-running postmaster exactly like an error does, and the cleanup is itself guarded so a failure to stop cannot mask the original error. --- @jdatcmd: design/ISSUE_432_PYTEST_HARNESS.md SECTION 8a WAS FALSE --------- He found row 7. I checked the other eight rather than fixing the one instance, and found a second: 9 named tests in section 8 ABSENT test_layer_fails_on_a_stale_library <- his finding ABSENT test_two_workers_get_different_clusters <- found by checking the rest Both properties ARE covered, under better names, and 8a now says which and why in a table rather than claiming "all of section 8 is implemented and green": row 7 -> test_build_refusal.py. The row described an mtime-vs-postmaster check that is near-vacuous alone, because every suite initdb's fresh so the postmaster always starts after the .so. What was needed is a refusal to measure a binary not built from this source at all. row 9 -> test_the_worker_owns_its_own_cluster. Asserts port == PORT_BASE + slot for its OWN worker; the mapping is injective, so every worker matching its own id implies no two share one -- and it is checkable from inside one worker, which the original phrasing was not. Section 8 is left AS WRITTEN and now says so: it is the plan from before the work, not an index of what exists. The count is corrected (74 in 6 files) and TESTS.md is named as the record, because TESTS.md is checked mechanically and this document is not. Taking his advice, the doc gate is NOT extended to design/ -- a design record describes decisions, and gating it puts a treadmill under prose that has no reason to track the tree. --- PROVED BY REMOVAL -------------------------------------------------------- unmutated f73e0013c0a9 18 passed fingerprint reverts to src-only 1a1715cafb47 2 failed (both objstore arms) make_cluster stops cleaning up 9cc1e703f77a 1 failed (the leak arm) restored f73e0013c0a9 byte-exact Each mutation asserted applied by md5 before the run, and each reddens exactly the arms that name it. WRITTEN TWICE, per jd's rule of 2026-09-09. test/pytest/test_build_refusal.py 3 arms, behavioural. 15 -> 18. test/selftest/380-the-pytest-cluster-helpers.sh 14 arms, static. The static half requires the GLOB rather than the name, and separately requires that "objstore" does NOT appear -- the only way to tell a derivation from a list that happens to be complete today. Verified: harness_selftest 315 passed + 0 failed + 0 unrunnable PASSED (301 before) docs_style 9 checks PASSED pytest 74 passed serial, 74 passed -n 4, marker cleared for each 74 passed with --pgc-expect-tests 74 shellcheck -S error -s bash test/*.sh test/selftest/*.sh exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Please hold this merge. @linuxhikerpm's review at 16:36 has three findings and all three are still live at
|
|
All three fixed and verified locally at Each was reproduced on this box before being fixed, not taken on the review's word:
Proved by removalEach mutation asserted applied by md5 before the run; each reddens exactly the arms that name it and no others. 14 new arms in Two things worth flagging beyond the fixesThe digest defect is the same class as one I fixed on #897 today, from the other direction: there the hash mixed in each file's bare name, so The stamp-key arms use fake What I need from youThe branch is approved, so I am still holding. My recommendation is unchanged and is option 2 from my earlier comment: dismiss your approval, I push, you re-review. These are three defects in the guarantee this PR is for, not additions to it. If you would rather merge One thing that has changed since you approved: #897 is now approved too, and I have asked you to land it first precisely because this PR has open findings and that one does not. That reverses which branch carries the stamp-write interlock — and the #898 side is the easier one, because with The pytest twin for these 14 arms is owed under the twin rule and lands with that rebase, since |
…at was false (commandprompt#432) THREE ITEMS, from two reviews I had not read when I asked for a re-review. I posted a closure table against @jdatcmd's 14:45 review while his 18:05 one and @linuxhikerpm's 16:38 one were both sitting on the PR. That is my error and it is the reason this commit exists rather than an approval. --- @linuxhikerpm 1: AN objstore/ EDIT WAS CERTIFIED AS ALREADY BUILT --------- Reproduced against the branch before fixing: objstore_before=2799803eaeac objstore_after=2799803eaeac builds=1 second=already-built source_fingerprint() read src/*.c, src/*.h, the top-level Makefile, *.control and *.sql. objstore/ is a SEPARATELY BUILT shared library the top-level Makefile reaches by recursion, so editing objstore/module.c left the hash unchanged and build_once treated a stale module as current. Its docstring said "Same input set as pgc_source_fingerprint in test/lib.sh", which was itself false -- and this is an INDEPENDENT implementation, so rebasing commandprompt#898 would not have fixed it. THAT is the argument for the two becoming one implementation rather than two that happen to agree. source_build_dirs() now derives the set by the rule the build itself follows: src/, plus any directory carrying its own Makefile. Naming objstore/ would fix today and fail the next time a module is added. AND A COLLISION THE GLOB DOES NOT FIX. The hash mixed in each file's bare NAME. With two build directories src/module.c and objstore/module.c become interchangeable -- swap their contents and the fingerprint does not move. It now mixes in the path relative to the tree. --- @linuxhikerpm 2: make_cluster LEAKED ITS TREE ON A FAILED SETUP ----------- make_cluster_error=RuntimeError new_roots=1 leaked=['/tmp/pgc-pytest-777-h3phhtxc'] root came from mkdtemp and then Cluster(), initdb(), start() and is_ours() ran with no cleanup guard. conftest.py cannot clean up after it, because `cluster, root = make_cluster(...)` never completes when the call raises. The HANDLED is_ours() path leaked too: it stopped the cluster and left the directory. Every exit that is not a successful return now stops whatever was started and removes the tree. It catches BaseException rather than Exception, because a KeyboardInterrupt during initdb leaks a datadir and a possibly-running postmaster exactly like an error does, and the cleanup is itself guarded so a failure to stop cannot mask the original error. --- @jdatcmd: design/ISSUE_432_PYTEST_HARNESS.md SECTION 8a WAS FALSE --------- He found row 7. I checked the other eight rather than fixing the one instance, and found a second: 9 named tests in section 8 ABSENT test_layer_fails_on_a_stale_library <- his finding ABSENT test_two_workers_get_different_clusters <- found by checking the rest Both properties ARE covered, under better names, and 8a now says which and why in a table rather than claiming "all of section 8 is implemented and green": row 7 -> test_build_refusal.py. The row described an mtime-vs-postmaster check that is near-vacuous alone, because every suite initdb's fresh so the postmaster always starts after the .so. What was needed is a refusal to measure a binary not built from this source at all. row 9 -> test_the_worker_owns_its_own_cluster. Asserts port == PORT_BASE + slot for its OWN worker; the mapping is injective, so every worker matching its own id implies no two share one -- and it is checkable from inside one worker, which the original phrasing was not. Section 8 is left AS WRITTEN and now says so: it is the plan from before the work, not an index of what exists. The count is corrected (74 in 6 files) and TESTS.md is named as the record, because TESTS.md is checked mechanically and this document is not. Taking his advice, the doc gate is NOT extended to design/ -- a design record describes decisions, and gating it puts a treadmill under prose that has no reason to track the tree. --- PROVED BY REMOVAL -------------------------------------------------------- unmutated f73e0013c0a9 18 passed fingerprint reverts to src-only 1a1715cafb47 2 failed (both objstore arms) make_cluster stops cleaning up 9cc1e703f77a 1 failed (the leak arm) restored f73e0013c0a9 byte-exact Each mutation asserted applied by md5 before the run, and each reddens exactly the arms that name it. WRITTEN TWICE, per jd's rule of 2026-09-09. test/pytest/test_build_refusal.py 3 arms, behavioural. 15 -> 18. test/selftest/380-the-pytest-cluster-helpers.sh 14 arms, static. The static half requires the GLOB rather than the name, and separately requires that "objstore" does NOT appear -- the only way to tell a derivation from a list that happens to be complete today. Verified: harness_selftest 315 passed + 0 failed + 0 unrunnable PASSED (301 before) docs_style 9 checks PASSED pytest 74 passed serial, 74 passed -n 4, marker cleared for each 74 passed with --pgc-expect-tests 74 shellcheck -S error -s bash test/*.sh test/selftest/*.sh exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
Two changes from the #902 review, both from OffgridwithJD. CONTEXT.md's twin rule now says to pin the SHA the twin was tested against rather than the branch name. Their argument is the one that convinced me: a branch name is not checkable later, and it is why they could verify my claim at all. The harness branch moved three times while the first twin was being written, and two of those moves changed its content -- so "blocked on #897" and "blocked on #897 at b785795" are different claims and only one can be falsified. Same reason a tag is read from the API rather than from a local ref, which I got wrong earlier today and filed a false issue over. The twin's header records that #897 moved a fourth time, to 9064a46, and DELIBERATELY DOES NOT UPDATE THE PIN. The point of a SHA is to say what was tested. What is recorded instead is why the pin still describes the current head, verified here rather than taken from the push notice: b785795 test/pytest tree = b20ad7e 9064a46 test/pytest tree = b20ad7e whole delta = 30 lines in one test/selftest/ file the harness never reads NOT CHANGED, deliberately: the five x86_64 build failures on this PR are the PGDG apt mirror, not this branch. The mirror is serving a Release file created at 17:16:59 alongside a component index last modified at 09:41:12, so the index cannot match the manifest describing it. Two attempts twenty minutes apart produced byte-identical hashes, which rules out a race. #898 at 6939bba and #897 at b785795 both went fully green before 17:16 and both #897 at 9064a46 and this branch fail after it, with #897's delta being thirty lines in a directory no build job reads. aarch64 passed all five majors throughout. Patching ci.yml around a mirror that is mid-sync would outlive the outage and get copied. docs_style.sh: 9 checks, PASSED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
Two changes from the #902 review, both from OffgridwithJD. CONTEXT.md's twin rule now says to pin the SHA the twin was tested against rather than the branch name. Their argument is the one that convinced me: a branch name is not checkable later, and it is why they could verify my claim at all. The harness branch moved three times while the first twin was being written, and two of those moves changed its content -- so "blocked on #897" and "blocked on #897 at b785795" are different claims and only one can be falsified. Same reason a tag is read from the API rather than from a local ref, which I got wrong earlier today and filed a false issue over. The twin's header records that #897 moved a fourth time, to 9064a46, and DELIBERATELY DOES NOT UPDATE THE PIN. The point of a SHA is to say what was tested. What is recorded instead is why the pin still describes the current head, verified here rather than taken from the push notice: b785795 test/pytest tree = b20ad7e 9064a46 test/pytest tree = b20ad7e whole delta = 30 lines in one test/selftest/ file the harness never reads NOT CHANGED, deliberately: the five x86_64 build failures on this PR are the PGDG apt mirror, not this branch. The mirror is serving a Release file created at 17:16:59 alongside a component index last modified at 09:41:12, so the index cannot match the manifest describing it. Two attempts twenty minutes apart produced byte-identical hashes, which rules out a race. #898 at 6939bba and #897 at b785795 both went fully green before 17:16 and both #897 at 9064a46 and this branch fail after it, with #897's delta being thirty lines in a directory no build job reads. aarch64 passed all five majors throughout. Patching ci.yml around a mirror that is mid-sync would outlive the outage and get copied. docs_style.sh: 9 checks, PASSED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
…mmandprompt#907) test/lib.sh and test/pytest/pgc_cluster.py each carried their own answer to "what was this binary built from". On 2026-09-09 the pair produced four defects between them -- two in each copy, and NOT ONE was found by whoever wrote that copy: objstore/*.c never walked python @linuxhikerpm, commandprompt#897 the bare NAME instead of the path python found while fixing the above `xargs -0 cat | md5sum`, no bounds shell @linuxhikerpm, commandprompt#898 each build dir's Makefile omitted python found while writing the twin The Python docstring asserted "the same input set as pgc_source_fingerprint in test/lib.sh" throughout all four. It was false when written and stayed false through two rounds of fixing. A prose claim of agreement is not a mechanism, and it is worse than silence because it is what stops the next person checking. Python, not shell, which is the opposite of what commandprompt#907 first proposed -------------------------------------------------------------------- jd's constraint decided it: the single implementation belongs in the more portable language. bash is largely a GNU thing; Python is present on FreeBSD and Windows where bash is not. lib.sh already requires bash, so calling a more portable interpreter from it cannot cost portability. My argument against this direction was that lib.sh invokes python3 zero times, so this escalates from "53 suites need it" to "every suite needs it at gate time". That is true and it is not a cost, for the reason above. Measured, expecting to report a subprocess penalty: shell, forking md5sum once per file 239 ms/call the module, one interpreter start 26 ms/call across 261 suites x 2 fingerprints 124 s -> 13 s The portable direction is also 9x faster. I had it backwards in both dimensions. A fifth defect, which unifying them found ------------------------------------------ `sort -z` orders by LOCALE COLLATION, and nothing in this harness pins a locale. The same tree fingerprinted two ways depending on whose machine it was: LC_ALL=C 6d122a7158d5 LC_ALL=en_US.UTF-8 0b59bd75fa4f en_US.UTF-8 is a common desktop default, so this is a developer stamping a tree and CI reading it back and calling the binary stale -- a false FATAL arriving from the environment rather than from the source. The module sorts BYTES, which is what LC_ALL=C produced and what every stamp already on disk was written with, so no existing stamp is invalidated. Arms in both harnesses. Equivalence, established rather than asserted ---------------------------------------------- A differential run of the module against the shell it replaces, over trees built to break the ways this pair has actually broken. 17 shapes, manifest AND fingerprint compared: the real source tree, minimal, a recursed module, a dir with sources but no Makefile, collation-sensitive names, a symlinked source file, a symlinked build directory, no src/, an empty tree, root .control and .sql, non-source files, spaces and punctuation, unicode, a Makefile at depth 3, a trailing slash, a /./ segment, five recursed modules AGREE=17 DIVERGE=0 Two of those are subtle enough to be worth naming. `find -type f` tests the LINK, so a symlinked source is not in the shell's manifest, while `pathlib.is_file()` FOLLOWS it and would have added one; the module excludes symlinks explicitly. And `find` does not descend a symlinked directory, so build dirs discovered through one differ -- which is why the module canonicalises the root first. The mechanism of two arms had to change with the implementation ---------------------------------------------------------------- The failed-digest arms in 340 and test_build_refusal.py stubbed `md5sum` on PATH. The digest is hashlib now, which no PATH can reach, so the stub would have left both arms GREEN while testing nothing -- the exact shape this corpus refuses. A real read failure needs a real reader who is denied, and root is denied nothing: chmod 000 is invisible to it. Measured before the arms were rewritten: as root 28a7149e07ae <- reads the mode-000 file regardless as postgres (empty) <- the failure the arm needs So the tree is built outside any mode-0700 directory and read by a second user, with a premise asserting that reader agrees with a privileged one WHILE nothing is denied -- otherwise the arm measures the user switch rather than the failure. Where no non-root user exists it records expect.cannot_run rather than passing. And the arm that would catch this issue recurring -------------------------------------------------- selftest 380's static guards follow the fingerprint to its new file, plus three new arms: neither caller may keep a private implementation, and the module may import nothing from test/pytest/. A static assertion of ABSENCE is the shape that most often cannot fail, so each was proved against the REAL files rather than only against fixtures -- a fixture proves the pattern matches something, not that the arm aimed at the real file would fire: pgc_cluster.py grows a private digest HELD lib.sh grows a private md5sum loop HELD the module imports from the pytest tree HELD harness_selftest 407 passed + 0 failed + 0 unrunnable, rc=0 pytest corpus 91 passed docs_style 9 checks PASSED shellcheck -S error clean Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…mmandprompt#907) test/lib.sh and test/pytest/pgc_cluster.py each carried their own answer to "what was this binary built from". On 2026-09-09 the pair produced four defects between them -- two in each copy, and NOT ONE was found by whoever wrote that copy: objstore/*.c never walked python @linuxhikerpm, commandprompt#897 the bare NAME instead of the path python found while fixing the above `xargs -0 cat | md5sum`, no bounds shell @linuxhikerpm, commandprompt#898 each build dir's Makefile omitted python found while writing the twin The Python docstring asserted "the same input set as pgc_source_fingerprint in test/lib.sh" throughout all four. It was false when written and stayed false through two rounds of fixing. A prose claim of agreement is not a mechanism, and it is worse than silence because it is what stops the next person checking. Python, not shell, which is the opposite of what commandprompt#907 first proposed -------------------------------------------------------------------- jd's constraint decided it: the single implementation belongs in the more portable language. bash is largely a GNU thing; Python is present on FreeBSD and Windows where bash is not. lib.sh already requires bash, so calling a more portable interpreter from it cannot cost portability. My argument against this direction was that lib.sh invokes python3 zero times, so this escalates from "53 suites need it" to "every suite needs it at gate time". That is true and it is not a cost, for the reason above. Measured, expecting to report a subprocess penalty: shell, forking md5sum once per file 239 ms/call the module, one interpreter start 26 ms/call across 261 suites x 2 fingerprints 124 s -> 13 s The portable direction is also 9x faster. I had it backwards in both dimensions. A fifth defect, which unifying them found ------------------------------------------ `sort -z` orders by LOCALE COLLATION, and nothing in this harness pins a locale. The same tree fingerprinted two ways depending on whose machine it was: LC_ALL=C 6d122a7158d5 LC_ALL=en_US.UTF-8 0b59bd75fa4f en_US.UTF-8 is a common desktop default, so this is a developer stamping a tree and CI reading it back and calling the binary stale -- a false FATAL arriving from the environment rather than from the source. The module sorts BYTES, which is what LC_ALL=C produced and what every stamp already on disk was written with, so no existing stamp is invalidated. Arms in both harnesses. Equivalence, established rather than asserted ---------------------------------------------- A differential run of the module against the shell it replaces, over trees built to break the ways this pair has actually broken. 17 shapes, manifest AND fingerprint compared: the real source tree, minimal, a recursed module, a dir with sources but no Makefile, collation-sensitive names, a symlinked source file, a symlinked build directory, no src/, an empty tree, root .control and .sql, non-source files, spaces and punctuation, unicode, a Makefile at depth 3, a trailing slash, a /./ segment, five recursed modules AGREE=17 DIVERGE=0 Two of those are subtle enough to be worth naming. `find -type f` tests the LINK, so a symlinked source is not in the shell's manifest, while `pathlib.is_file()` FOLLOWS it and would have added one; the module excludes symlinks explicitly. And `find` does not descend a symlinked directory, so build dirs discovered through one differ -- which is why the module canonicalises the root first. The mechanism of two arms had to change with the implementation ---------------------------------------------------------------- The failed-digest arms in 340 and test_build_refusal.py stubbed `md5sum` on PATH. The digest is hashlib now, which no PATH can reach, so the stub would have left both arms GREEN while testing nothing -- the exact shape this corpus refuses. A real read failure needs a real reader who is denied, and root is denied nothing: chmod 000 is invisible to it. Measured before the arms were rewritten: as root 28a7149e07ae <- reads the mode-000 file regardless as postgres (empty) <- the failure the arm needs So the tree is built outside any mode-0700 directory and read by a second user, with a premise asserting that reader agrees with a privileged one WHILE nothing is denied -- otherwise the arm measures the user switch rather than the failure. Where no non-root user exists it records expect.cannot_run rather than passing. And the arm that would catch this issue recurring -------------------------------------------------- selftest 380's static guards follow the fingerprint to its new file, plus three new arms: neither caller may keep a private implementation, and the module may import nothing from test/pytest/. A static assertion of ABSENCE is the shape that most often cannot fail, so each was proved against the REAL files rather than only against fixtures -- a fixture proves the pattern matches something, not that the arm aimed at the real file would fire: pgc_cluster.py grows a private digest HELD lib.sh grows a private md5sum loop HELD the module imports from the pytest tree HELD harness_selftest 407 passed + 0 failed + 0 unrunnable, rc=0 pytest corpus 91 passed docs_style 9 checks PASSED shellcheck -S error clean Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
3 participants
The matrix builds once per major and then runs every suite with
PGC_SKIP_BUILD=1. Nothing checked that the binary those suites measured came from the tree under test. This adds that check, at the controller, so a stale.socannot be measured and nothing has to rebuild per test.Two failure modes, both of which have bitten this project before, and both now FATAL rather than advisory.
1. The binary was not built from this source
The controller records a fingerprint of the build inputs after a successful install. Every suite compares it, whether that suite built or skipped.
Proved by removal, with controls on both sides:
The red arm reports:
It names both fingerprints, because "stale" without the two values leaves the reader unable to tell a real drift from a broken fingerprint.
2. The server predates the binary
A
make installdoes not reload anything.shared_preload_librariesmaps the library at postmaster start, so a reinstall under a running server leaves the backends executing the old code while the file on disk is new. The check compares the.somtime againstpg_postmaster_start_time():and on the bad verdict says what to do rather than only what is wrong:
unknownis not a failure, deliberatelySomeone who ran
make installby hand has no stamp. Refusing would break a documented workflow, so that case printsThe point is that it says which question was not answered, rather than printing nothing and letting a reader assume the check passed. A silent third state is how a verdict becomes lossy.
The verdict functions are pure, and tested as such
pgc_freshness_verdictandpgc_running_binary_verdicttake strings and returnfresh/stale/unknownandfresh/predates/unknown. They touch no filesystem, sotest/selftest/340-the-binary-must-be-built-from.shtests them directly — 16 arms including both empty inputs, a non-numeric epoch, equal timestamps on the exact boundary, and fingerprint sensitivity to each input class.harness_selftestgoes from 261 checks to 277.Two defects this found in itself
The stamp was written in the wrong branch first. My initial version wrote it in the skip-build path, which made the check tautological: it recomputed the fingerprint of the source it had just read and reported "matches" on edited source. My own red arm caught it. The stamp is now written only after a successful install; the verify runs always.
The controller swallowed its own failure. The stamp write was
... || true. If it failed, every suite in the batch would reportfreshness UNVERIFIEDand the controller arm would silently stop being a controller arm — the exact state this exists to prevent, with nothing saying so. It now prints what a failure means for the run below it.Verification
Full matrix on both majors at
0332527:A full matrix is the right bar here and there was no shortcut available: this changes
test/lib.sh, which every suite reads.The check was observed working inside that matrix, not only in isolation. Sampling the live PG19 per-suite logs three times during the run:
The five that report nothing are accounted for rather than assumed:
audit,bench_guards,concurrencyanddocs_stylenever callpgc_setup, anddecode_interruptsis a static source-analysis suite whose only occurrence ofpgc_setupis the comment "No cluster needed; pgc_setup is skipped deliberately." — my firstgrep -ccounted that comment as a call..gitignoregains.pgc_source_stamp.*beside.pgc_built_for_major, which is the same kind of file for the same reason. Verified the new rule is the one matching — the file was not ignored before, andgit check-ignore -vnow names.gitignore:10— rather than assuming my rule was what caught it.